Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minor edits #82

Closed
wants to merge 11 commits into from
Closed

Minor edits #82

wants to merge 11 commits into from

Conversation

dericed
Copy link
Contributor

@dericed dericed commented Jan 2, 2018

No description provided.

retokromer
retokromer reviewed Jan 2, 2018
View reviewed changes
ffv1.md Outdated
@@ -160,13 +160,13 @@ Several components of FFV1 are described in this document using pseudo-code. Not

### NumBytes

NumBytes is a non-negative integer that expresses the size in 8-bit octets of particular FFV1 components such as the Configuration Record and Frame. FFV1 relies on its container to store the NumBytes values, see [the section on the `Mapping FFV1 into Containers`](#mapping-ffv1-into-containers).
`NumBytes` is a non-negative integer that expresses the size in 8-bit octets of particular FFV1 components such as the `Configuration Record` and Frame. FFV1 relies on its container to store the `NumBytes` values, see [the section on the `Mapping FFV1 into Containers`](#mapping-ffv1-into-containers).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say either `Frame` or frame.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frame is defined:
https://github.com/FFmpeg/FFV1/blob/master/ffv1.md#frame

so +1 for Frame

retokromer
retokromer reviewed Jan 2, 2018
View reviewed changes
ffv1.md Outdated
@@ -593,7 +593,7 @@ The alternative state transition table has been built using iterative minimizati

### Huffman coding mode

This coding mode uses Golomb Rice codes. The VLC code is split into 2 parts, the prefix stores the most significant bits, the suffix stores the k least significant bits or stores the whole number in the ESC case. The end of the bitstream (of the frame) is filled with 0-bits until that the bitstream contains a multiple of 8 bits.
This coding mode uses Golomb Rice codes. The VLC code is split into 2 parts, the prefix stores the most significant bits, the suffix stores the k least significant bits or stores the whole number in the ESC case. The end of the bitstream of the frame is filled with 0-bits until that the bitstream contains a multiple of 8 bits.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"0-bits" and "until that" sounds bizarre to me (not English native): possibly "filled with 0s until the bitstream contains"?

retokromer
retokromer reviewed Jan 2, 2018
View reviewed changes
Copy link
Contributor

@retokromer retokromer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments, LGTM.

@dericed
Copy link
Contributor Author

dericed commented Jan 2, 2018

I suggest to address the comments in a subsequent PR since they are not about the changes of the PR.

@JeromeMartinez
Copy link
Contributor

IMO the first one is related, as you put quotes for all items having a dedicated chapter for bitstream description (e.g. Configuration Record) except Frame. Configuration Record and Frame are linked (both are at the top level in a container)

@dericed
Copy link
Contributor Author

dericed commented Jan 2, 2018

OK Frame is updated in 5976f83. There are a few uses of the term "frame" that don't refer to the FFV1 context of the term, so some of backtick quoted and some not.

JeromeMartinez
JeromeMartinez approved these changes Jan 2, 2018
View reviewed changes
Copy link
Contributor

@JeromeMartinez JeromeMartinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit reluctant for 'Frames' (plural) as it is not explicitly related to 'Frame' (singular) but IMO does not prevent merge of this PR.
(a later PR could e.g. change 'Frames' to "instances of 'Frame'")

@dericed
Copy link
Contributor Author

dericed commented Jan 2, 2018

updated to avoid plural of Frame

@michaelni
Copy link
Member

Please use commit messages that do not need to be edited
4f6df66 for example says "plz review", i cannot push this as is and thus cannot just merge this without changes

-The bitstream contains 1 or more Quantization Table Sets.
+The FFV1 bitstream contains 1 or more Quantization Table Sets.
This adds trailing whitespace, i suggest you configure your editor to never add trailing whitespace

@dericed
Copy link
Contributor Author

dericed commented Jan 2, 2018

@michaelni, the plz review commit message is removed.

The patch preserves the trailing whitespace before and after the edit. In markdown when a line ends with two-spaces (as it does here and a few other places), then a line break is inserted. Personally I'd prefer to use an actual line break rather than the two-space line endings. This is the topic of another PR at #77.

@michaelni
Copy link
Member

Merged,
i had to rebase it as it wasnt based on latest master HEAD

thanks

@michaelni michaelni closed this Jan 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JeromeMartinez JeromeMartinez JeromeMartinez approved these changes

@retokromer retokromer retokromer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dericed @JeromeMartinez @michaelni @retokromer